Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware, @elintul, and @giladchase)

crates/mempool/src/mempool_test.rs line 249 at r1 (raw file):

    let mempool_state = MempoolState::new(pool_txs, queue_txs);

    mempool_state.assert_eq_mempool_state(&mempool);

Suggestion:

    let pool_txs = [input_big_tip_small_hash.tx, input_small_tip_big_hash.tx];
    
    let mempool_state = MempoolState::new(pool_txs, queue_txs);
    mempool_state.assert_eq_mempool_state(&mempool);

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @elintul, @giladchase, and @MohammadNassar1)

crates/mempool/src/mempool_test.rs line 249 at r1 (raw file):

    let mempool_state = MempoolState::new(pool_txs, queue_txs);

    mempool_state.assert_eq_mempool_state(&mempool);

I think the first option is more readable

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @giladchase)

crates/mempool/src/mempool_test.rs line 256 at r2 (raw file):

#[rstest]
fn test_tip_priority_over_tx_hash(mut mempool: Mempool) {

What we are testing here?
I don't see get_txs method.

Code quote:

n test_tip_priority_over_tx_hash(

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware, @elintul, and @giladchase)

crates/mempool/src/mempool_test.rs line 256 at r2 (raw file):

Please ignore.
The test tests that in tx_queue there are two transactions, and not one, as they have different tx hash.

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware, @elintul, and @MohammadNassar1)

crates/mempool/src/mempool_test.rs line 273 at r3 (raw file):

        TransactionReference::new(&input_small_tip_big_hash.tx),
    ];
    let expected_pool_txs = [input_big_tip_small_hash.tx, input_small_tip_big_hash.tx];

Unnecessary in this test, there are only 2 txs and both are in the queue.

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @elintul, @giladchase, and @MohammadNassar1)

crates/mempool/src/mempool_test.rs line 273 at r3 (raw file):

Done.

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware, @elintul, and @MohammadNassar1)

crates/mempool/src/mempool_test.rs line 44 at r4 (raw file):

        let tx_queue: TransactionQueue = queue_txs.into_iter().collect();
        MempoolState { tx_pool: Default::default(), tx_queue }
    }

This is a good attempt for what we have now, and i nearly LGTMed it for now, but this might be hard to debug when a test fails: one would typically print the mempool contents when debugging, which will have an empty pool, and that can be very confusing to anyone not intimately aware of this util.

So I think a more strongly typed approach is in order, even at this stage.
See if this works:

#[derive(Debug)]
struct MempoolState<T> {
    tx_pool: Option<TransactionPool>,
    tx_queue: Option<TransactionQueue>,
    _phantom: PhantomData<T>,
}

#[derive(Debug)]
struct FullState;
#[derive(Debug)]
struct PartialState;

impl MempoolState<FullState> {
    fn new<P, Q>(pool_txs: P, queue_txs: Q) -> Self
    where
        P: IntoIterator<Item = ThinTransaction>,
        Q: IntoIterator<Item = TransactionReference>,
    {
        Self {
            tx_pool: Some(pool_txs.into_iter().collect()),
            tx_queue: Some(queue_txs.into_iter().collect()),
            _phantom: PhantomData,
        }
    }
}

impl Default for MempoolState<PartialState> {
    fn default() -> Self {
        Self { tx_pool: None, tx_queue: None, _phantom: PhantomData }
    }
}

impl MempoolState<PartialState> {
    fn empty() -> Self {
        Default::default()
    }

    fn _with_pool<P>(mut self, pool_txs: P) -> Self
    where
        P: IntoIterator<Item = ThinTransaction>,
    {
        self.tx_pool = Some(pool_txs.into_iter().collect());
        self
    }

    fn with_queue<Q>(mut self, queue_txs: Q) -> Self
    where
        Q: IntoIterator<Item = TransactionReference>,
    {
        self.tx_queue = Some(queue_txs.into_iter().collect());
        self
    }
}

impl<T> MempoolState<T> {
    fn assert_eq_mempool_state(&self, mempool: &Mempool) {
        self.assert_eq_pool_state(mempool);
        self.assert_eq_queue_state(mempool);
    }

    fn assert_eq_pool_state(&self, mempool: &Mempool) {
        assert_eq!(self.tx_pool.as_ref().unwrap(), &mempool.tx_pool);
    }

    fn assert_eq_queue_state(&self, mempool: &Mempool) {
        assert_eq!(self.tx_queue.as_ref().unwrap(), &mempool.tx_queue);
    }
}

If it works, and it is clear how to use, use it instead, otherwise lets try to think of another solution that will print None when debugging these kinds of tests, but that doesn't incur boilerplate during the tests, and that can also scale to the remaining components we'll soon need here (like the so-called "staging area")

Code quote:

    fn new_with_queue_txs_default_queue<QueueTxs>(queue_txs: QueueTxs) -> Self
    where
        QueueTxs: IntoIterator<Item = TransactionReference>,
    {
        let tx_queue: TransactionQueue = queue_txs.into_iter().collect();
        MempoolState { tx_pool: Default::default(), tx_queue }
    }

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @giladchase and @MohammadNassar1)

crates/mempool/src/mempool_test.rs line 44 at r4 (raw file):

Nice! What's the use of PhantomData?

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @elintul and @MohammadNassar1)

crates/mempool/src/mempool_test.rs line 44 at r4 (raw file):

Because we don't really use FullState and PartialState, they are just "markers" which are just some type artifact that allows the compiler to treat MempoolState<FullState> and MempoolState<PartialState> as separate types.

Without the phantomdata field, the compiler would have whined about there being an unused generic T in the definition of Mempool.
It is also optimized for this specific purpose, and in particular is called a "zero sized type", because it has 0 size on the stack. In other words, when a mempool instance is initialized and put on the stack, only the pool/queue fields take up space, whereas the phantomdata field doesn't appear.

However, if you dbg!(mempool_state), you will see MempoolState or MempoolState (depending on which one you printed), because the compiler treats these as separate types

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @giladchase and @MohammadNassar1)

crates/mempool/src/mempool_test.rs line 44 at r4 (raw file):

Very cool, thanks. Let's try to imp. this in a separate PR.

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ayeletstarkware)

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware)

crates/mempool/src/mempool_test.rs line 351 at r6 (raw file):

    add_tx(&mut mempool, &input_small_tip_big_hash);

    // Assert: ensure that the transaction with the higher tip is sequenced first.

Suggestion:

prioritized higher

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ayeletstarkware)

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ayeletstarkware)

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ayeletstarkware)